Notifications for remote shares by nickvergessen · Pull Request #22253 · owncloud/core

nickvergessen · 2016-02-09T16:23:46Z

Functionally only visible with owncloud/notifications#62

Fix #19587

mention-bot · 2016-02-09T16:23:48Z

By analyzing the blame information on this pull request, we identified @schiesbn, @rullzer and @icewind1991 to be potential reviewers

PVince81 · 2016-02-09T16:26:47Z

you could put that in an else block (optional)

PVince81 · 2016-02-09T16:28:54Z

Code looks good, as discussed, saw it working on your screen 👍

rullzer · 2016-02-09T16:39:21Z

Works!
Code looks good. 👍

PVince81 · 2016-02-09T21:02:53Z

Related Oracle fail:

11:55:26 1) Test_Files_Sharing_S2S_OCS_API::testCreateShare
11:55:26 Failed asserting that false is true.
11:55:26 
11:55:26 /ssd/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/apps/files_sharing/tests/server2server.php:112

PVince81 · 2016-02-09T21:52:02Z

Could it be the lastInsertId https://github.com/owncloud/core/pull/22253/files#diff-095b5f4e7ee9665a242886c86670b4b8R84 ?

Oracle is known to dislike those...

PVince81 · 2016-02-09T22:07:23Z

might be possible to get the share item from the shared storage, I see that addShare returns a IMountPoint instance:
$mount->getStorage()->getShare()['id'] (I didn't test, just guessed from the code)

Its null here

External shares don't have that. I believe we just mount them as 'true' webdav mounts. @schiesbn

Couldn't we just add getShareId to the ISharedStorge interface? The data is there when we construct the storage..

aaah no because we first insert it temp... mmm stupid oci...

@rullzer ISharedStorage already has getShare() which returns the old-school share array that has the id in it

https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/isharedstorage.php <-- well that is empty ;)

But the thing is that we don't get a mountpoint on the original insert. So there is no way to get an id from it...

Why would it be empty ? Does it mean it's a special case where no notification should be created ?
Or is it only that unit test that is weird ? (it creates a share with null, is that even a thing ?)

Woops, the table name should not be quoted in lastInsertId, I guess that will solve it.

It's null, when we don't mount the remote share now. This is when someone inserts a cloudId in the share sidebar.
In this case there needs to be a "accept please" message, which is exactly the case which we want.

But all good with my fix I think

nickvergessen · 2016-02-10T07:40:05Z

Wrong time for this joke oracle:

08:38:08 ownCloud is not installed - only a limited number of commands are available
08:38:09 Oracle connection could not be established
08:38:09  -> ORA-01017: invalid username/password; logon denied Check environment: ORACLE_HOME=/usr/lib/oracle/xe/app/oracle/product/10.2.0/server ORACLE_SID=XE LD_LIBRARY_PATH=/usr/lib/oracle/xe/app/oracle/product/10.2.0/server/lib: NLS_LANG=AMERICAN_AMERICA.AL32UTF8 tnsnames.ora is readable

repushing

…erly" This reverts commit 6bc93c7. Conflicts: apps/files_sharing/lib/external/manager.php

nickvergessen · 2016-02-10T08:41:32Z

postgres failure is the known hick up where the "md5" is unequal to the string object "md5"

PVince81 · 2016-02-10T08:44:17Z

👍

Notifications for remote shares

lock · 2019-08-07T19:02:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

nickvergessen added 3 - To Review feature:federated-cloud-sharing feature:notifications labels Feb 9, 2016

nickvergessen added this to the 9.0-current milestone Feb 9, 2016

DeepDiver1975 added the in progress label Feb 9, 2016

PVince81 reviewed Feb 9, 2016
View reviewed changes

Comment thread apps/files_sharing/js/external.js Outdated

Copy link
Copy Markdown

Contributor

PVince81 Feb 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could put that in an else block (optional)

nickvergessen added 4 - To release and removed 3 - To Review labels Feb 9, 2016

PVince81 added 2 - Developing and removed 4 - To release labels Feb 9, 2016

PVince81 reviewed Feb 9, 2016
View reviewed changes

nickvergessen force-pushed the notifications-for-remote-shares branch from cf13477 to 7b40bf1 Compare February 10, 2016 07:26

nickvergessen added 9 commits February 10, 2016 08:40

Revert "Disable the remote sharing notifications until they work prop…

3ff88c8

…erly" This reverts commit 6bc93c7. Conflicts: apps/files_sharing/lib/external/manager.php

Fix the notification API usage

a4a7cf4

Bring the messages inline

49dd693

Fix action paths

cb8024c

Make sure the share ID is an integer

31cf3b8

Create the actions with the correct ID

c769f57

Mark the accept button as primary

65e1e4a

Listen to the notification event to reload the file list

75d552b

Fix oracle by using less quotes

fa89376

nickvergessen force-pushed the notifications-for-remote-shares branch from 7b40bf1 to fa89376 Compare February 10, 2016 07:40

nickvergessen added the 4 - To release label Feb 10, 2016

nickvergessen removed the 2 - Developing label Feb 10, 2016

DeepDiver1975 added a commit that referenced this pull request Feb 10, 2016

Merge pull request #22253 from owncloud/notifications-for-remote-shares

9a2c517

Notifications for remote shares

DeepDiver1975 merged commit 9a2c517 into master Feb 10, 2016

DeepDiver1975 deleted the notifications-for-remote-shares branch February 10, 2016 09:06

DeepDiver1975 removed the in progress label Feb 10, 2016

lock Bot locked as resolved and limited conversation to collaborators Aug 7, 2019

Conversation

nickvergessen commented Feb 9, 2016

Uh oh!

mention-bot commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Feb 9, 2016

Uh oh!

rullzer commented Feb 9, 2016

Uh oh!

PVince81 commented Feb 9, 2016

Uh oh!

PVince81 commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Feb 10, 2016

Uh oh!

nickvergessen commented Feb 10, 2016

Uh oh!

PVince81 commented Feb 10, 2016

Uh oh!

lock Bot commented Aug 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants